Skip to content

Fix/documentation#38

Merged
dcuestam merged 10 commits intomasterfrom
fix/documentation
Sep 10, 2018
Merged

Fix/documentation#38
dcuestam merged 10 commits intomasterfrom
fix/documentation

Conversation

@dcuestam
Copy link
Copy Markdown
Contributor

@dcuestam dcuestam commented Sep 7, 2018

Make sure you have checked all steps below.

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits have been squashed if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

License

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.

otorreno
otorreno previously approved these changes Sep 7, 2018
Copy link
Copy Markdown
Contributor

@otorreno otorreno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 7, 2018

Codecov Report

Merging #38 into master will increase coverage by 0.35%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   94.09%   94.45%   +0.35%     
==========================================
  Files          13       13              
  Lines         847      847              
==========================================
+ Hits          797      800       +3     
+ Misses         50       47       -3
Flag Coverage Δ
#Linux 93.15% <ø> (?)
#MacOS 92.91% <ø> (?)
#Windows 94.09% <ø> (ø) ⬆️
Impacted Files Coverage Δ
khiva/matrix.py 100% <ø> (ø) ⬆️
khiva/library.py 91.8% <0%> (+4.91%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e491d35...f82ba78. Read the comment docs.

a += b
np.testing.assert_array_equal(a.to_numpy(), np.array([2, 4, 6, 8]))

def testIaddSelfArray(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

David, delete this test, as it may collision with PR 37.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about that, I will pull the changes and fix it when PR 37 is merged. Right now this branch have a pull from the branch of PR 37.

Comment thread khiva/matrix.py Outdated
""" Stomp algorithm to calculate the matrix profile between `t` and itself using a subsequence length of `m`.
This method filters the trivial matches.


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra empty line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Comment thread khiva/matrix.py
@@ -77,6 +77,10 @@ def find_best_n_motifs(profile, index, m, n, self_join=False):
def stomp(first_time_series, second_time_series, subsequence_length):
""" Stomp algorithm to calculate the matrix profile between `ta` and `tb` using a subsequence length of `m`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this method filter trivial matches? if so, please add, "This method filters the trivial matches.", as the one below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method doesn't filter trivial matches as it consider that both time series are different.

avilchess
avilchess previously approved these changes Sep 7, 2018
@dcuestam dcuestam merged commit 6ab405f into master Sep 10, 2018
@dcuestam dcuestam deleted the fix/documentation branch September 10, 2018 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants